[CSS-in-JS] Convert `EuiAvatar` by thompsongl · Pull Request #5670 · elastic/eui

thompsongl · 2022-03-01T16:30:30Z

Summary

Converts EuiAvatar styles from Sass to Emotion
Establishes a pattern for using array syntax inside of css prop values for better class name output ([CSS-in-JS] Convert EuiAvatar #5670 (comment))
Establishes a pattern for components with variant, stateful, and subcomopnent styles

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in Chrome, Safari, Edge, and Firefox
Checked Code Sandbox works for any docs examples
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately
Performance testing

kibanamachine · 2022-03-01T17:06:01Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

thompsongl · 2022-03-01T18:53:53Z

src/components/comment_list/__snapshots__/comment.test.tsx.snap

+  -webkit-box-pack: center;
+  -ms-flex-pack: center;
+  -webkit-justify-content: center;
+  justify-content: center;


Doesn't seem like we should need all this vendor prefixing. Investigating.
https://caniuse.com/flexbox

Stylis is tracking; not sure it's worth doing something non-standard when it comes to prefixing.

Just noting, also, that updating .browserslistrc to eliminate Safari 7 does not change the generated vendor prefixes.

thompsongl · 2022-03-01T18:56:45Z

src/components/avatar/avatar.tsx

+      css={[
+        styles.euiAvatar,
+        styles[size],
+        styles[type],
+        isDisabled && styles.isDisabled,
+        color === 'plain' && styles.plain,
+      ]}


Interestingly, using an array inside the css prop will result in the component name getting used in the resulting class name. Definitely not documented anywhere I can see.
So just using "labelFormat": "[local]" config, we get a class name like css-{hash}-EuiAvatar. Or css-{hash}-s-space-EuiAvatar if other variants are in play. This seems good!

Also, it's not important for this component, but order matters when composing styles in a css prop array: https://emotion.sh/docs/composition

For instance, if we were setting background-color in the isDisabled styles, the plain styles would "win" the specificity battle. This is largely helpful for our purposes but different from how the CSS specificity rules.

kibanamachine · 2022-03-02T18:23:53Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-03T20:30:15Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

cee-chen · 2022-03-03T22:21:48Z

Not sure if it matters, but should the CSS name still be mangled/uglified on the PR staging link? Here's what I'm seeing on https://eui.elastic.co/pr_5670/#/display/avatar:

Mostly asking because from my understanding on the other PR, we were not mangling strings on prod? Or am I getting confused and that setting was on the EuiMark PR only?

src/components/avatar/avatar.styles.ts

thompsongl · 2022-03-03T23:09:33Z

Or am I getting confused and that setting was on the EuiMark PR only?

Whoops, sorry for the confusion. This PR is missing the "autoLabel": "always" config that is present in the EuiMark PR.

src/global_styling/mixins/_size.ts

kibanamachine · 2022-03-04T20:56:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

cchaos

A whole lotta comments/questions, but in general I love it. We can go over some of the more discussion-y questions in the sync.

src-docs/src/views/theme/_json/eui_legacy_dark.json

src/components/avatar/__snapshots__/avatar.test.tsx.snap

src/global_styling/mixins/_size.ts

src/components/avatar/avatar.tsx

src/components/avatar/avatar.styles.ts

src/components/avatar/avatar.tsx

src/components/avatar/avatar.styles.ts

kibanamachine · 2022-03-10T20:48:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-15T17:45:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-16T22:07:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-21T17:20:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-21T20:19:40Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

kibanamachine · 2022-03-22T20:42:29Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

thompsongl · 2022-03-22T21:31:18Z

[From the last sync]
Can we generate multiple classes per component to eliminate duplication?

No. Unfortunately that just isn't how emotion (or styled-components) works. The whole point is to eliminate naming collisions and specificity overrides. They solved it by creating a unique class per style instance. Even if we used a workaround to pass classes to className the emotion serializer still process and combines the style rules. Luckily, it's fast, and keeps track of composition so that all small space avatars on the page will reuse the same class.

Can we now move the style into css directly so we start eliminating inline styles?

We'll want to keep some inline styles for dynamic styles like generating a color based on content. This will dramatically reduce the number of generated classes.

This does kind of mess with our BEM naming convention though. Do we care?

Perhaps we have a separate function for subcomponents. EuiAvatarIcon doesn't actually have styles, so this PR isn't the appropriate place to explore.

But either Emotion or something else is actually doing the calculation so it renders as:
font-size: calc(14.4px);
Have we actually tested trying to do math on px values to see how Emotion specifically handles it? This could be a huge game-changer if we don't need calc().

Math on px values does not work (we expected this). After the refactor to dedupe the sizing styles, the output is font-size: calc(16px * .9);. I haven't been able to find why we go 14.4px previously.

cchaos · 2022-03-30T19:16:26Z

@thompsongl I'm ready to do a final review on this but can you pull in main and update the snapshots?

kibanamachine · 2022-03-30T21:00:54Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

It removed the size `-m` from the class ☹️

Fixes class name output but is it worth it?

Adds a typically unecessary extra css`` wrapper but keeps the keys

cchaos

I pushed 3 commits which are 3 attempts to DRY out the sizing styles. The first was the most intuitive to me, but resulted in the size value (-m) no longer being output to the class name. So the next two were other attempts. I finalized on the third but the con is that it results in multiple css wrappers. I don't know what that might do to performance (if any) but it kind of hurts readability just slightly.

src-docs/src/views/theme/_json/eui_theme_dark.json

src/components/avatar/avatar.tsx

kibanamachine · 2022-03-30T22:42:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

thompsongl · 2022-04-05T14:21:37Z

Sorry for the delayed responses; I completely missed the notification!

I finalized on the third but the con is that it results in multiple css wrappers. I don't know what that might do to performance (if any) but it kind of hurts readability just slightly.

I think we can remove one layer of css, but I'm looking now. (See #5670 (comment))

thompsongl · 2022-04-05T14:56:48Z

src/components/avatar/avatar.styles.ts

+  s: css(
+    _avatarSize({
+      size: euiTheme.size.l,
+      fontSize: euiTheme.size.m,
+    })
+  ),


The css method can be called as just a normal function for cases like this. And _avatarSize can just return a normal string that will be accepted.
🎉

Lets get some docs written down about this before we forget 🤔

kibanamachine · 2022-04-05T15:35:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

cchaos

🙌 Thank you for that wiki addition!

kibanamachine · 2022-04-05T16:40:32Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5670/

thompsongl mentioned this pull request Mar 1, 2022

[CSS-in-JS] Component conversions #5432

Closed

12 tasks

thompsongl commented Mar 1, 2022

View reviewed changes

thompsongl mentioned this pull request Mar 1, 2022

[CSS-in-JS] Convert EuiMark #4575

Merged

7 tasks

thompsongl added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Mar 2, 2022

thompsongl marked this pull request as ready for review March 2, 2022 20:04

thompsongl mentioned this pull request Mar 3, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

thompsongl force-pushed the css-in-js/euiavatar branch from 576d747 to 06694a1 Compare March 3, 2022 19:58

thompsongl changed the base branch from css-in-js/conversions to main March 3, 2022 19:58

thompsongl added breaking change PRs with breaking changes. (Don't delete - used for automation) and removed skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) labels Mar 3, 2022

cee-chen reviewed Mar 3, 2022

View reviewed changes

src/components/avatar/avatar.styles.ts Outdated Show resolved Hide resolved

snide reviewed Mar 4, 2022

View reviewed changes

src/global_styling/mixins/_size.ts Outdated Show resolved Hide resolved

cchaos reviewed Mar 9, 2022

View reviewed changes

thompsongl force-pushed the css-in-js/euiavatar branch from 779634c to ea064fa Compare March 10, 2022 20:19

thompsongl removed the breaking change PRs with breaking changes. (Don't delete - used for automation) label Mar 10, 2022

thompsongl force-pushed the css-in-js/euiavatar branch from ea064fa to 7d3f7e9 Compare March 15, 2022 17:11

thompsongl added 4 commits March 21, 2022 11:30

style with emotion

8765540

remove theme vars

fd16ab3

snapshot updates

22b95b3

remove legacy theme styles and override (faux inner border)

8457815

thompsongl force-pushed the css-in-js/euiavatar branch from 872615d to 020d12a Compare March 21, 2022 16:37

fix CL

ca83947

Merge branch 'main' into css-in-js/euiavatar

09826d7

thompsongl requested a review from cchaos March 23, 2022 22:49

thompsongl added 3 commits March 30, 2022 15:18

Merge branch 'main' into css-in-js/euiavatar

53c401a

add renderWithStyles test; update snapshots

d6a6147

wiki

7464aa1

cchaos added 3 commits March 30, 2022 17:48

Tried: Moved sizing to an external function

04a6a37

It removed the size `-m` from the class ☹️

Option 2: just size

23b7d2a

Fixes class name output but is it worth it?

Option 3: Ensure css`` is the value of the key

df12df7

Adds a typically unecessary extra css`` wrapper but keeps the keys

cchaos reviewed Mar 30, 2022

View reviewed changes

src-docs/src/views/theme/_json/eui_theme_dark.json Show resolved Hide resolved

src/components/avatar/avatar.tsx Outdated Show resolved Hide resolved

src/components/avatar/avatar.tsx Show resolved Hide resolved

cchaos mentioned this pull request Mar 31, 2022

Add EuiTimeline #5730

Merged

11 tasks

refactor sizes

7654f01

thompsongl commented Apr 5, 2022

View reviewed changes

remove isPlain className

f836efe

add wiki

861f86a

cchaos approved these changes Apr 5, 2022

View reviewed changes

thompsongl merged commit e85c6e7 into elastic:main Apr 5, 2022

cee-chen mentioned this pull request Apr 6, 2022

Upgrade EUI to v54.0.0 elastic/kibana#129653

Merged

cee-chen mentioned this pull request Oct 9, 2023

[Emotion] Reduce CSS browser prefixing to supported browsers only #7272

Merged

5 tasks

Conversation

thompsongl commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Mar 1, 2022

Uh oh!

thompsongl Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

thompsongl Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

thompsongl Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

thompsongl Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

thompsongl Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 2, 2022

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

cee-chen commented Mar 3, 2022

Uh oh!

Uh oh!

thompsongl commented Mar 3, 2022

Uh oh!

Uh oh!

kibanamachine commented Mar 4, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Mar 10, 2022

Uh oh!

kibanamachine commented Mar 15, 2022

Uh oh!

kibanamachine commented Mar 16, 2022

Uh oh!

kibanamachine commented Mar 21, 2022

Uh oh!

kibanamachine commented Mar 21, 2022

Uh oh!

kibanamachine commented Mar 22, 2022

Uh oh!

thompsongl commented Mar 22, 2022

Uh oh!

cchaos commented Mar 30, 2022

Uh oh!

kibanamachine commented Mar 30, 2022

Uh oh!

cchaos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Mar 30, 2022

Uh oh!

thompsongl commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thompsongl Apr 5, 2022

Choose a reason for hiding this comment

thompsongl commented Mar 1, 2022 •

edited

Loading

cchaos left a comment •

edited

Loading

thompsongl commented Apr 5, 2022 •

edited

Loading